-
-
Notifications
You must be signed in to change notification settings - Fork 274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coins deprecation #16131
Coins deprecation #16131
Conversation
🚀 Expo preview is ready!
|
3e2ce44
to
2a3c207
Compare
@@ -78,9 +58,7 @@ export const filterBlacklistedNetworks = ( | |||
); | |||
|
|||
export const portfolioTrackerMainnets = sortNetworks( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I see correctly, if we remove the portfolio blacklist, we can remove portfolioTrackerMainnets
and getPortfolioTrackerTestnets
completely and use networkSymbolsWhitelistMap.mainnets
and networkSymbolsWhitelistMap.testnets
instead, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to create issue for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In here - #16683
@@ -18,12 +18,7 @@ type NetworkSymbol = | |||
| 'etc' | |||
| 'xrp' | |||
| 'bch' | |||
| 'btg' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep those in this migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. maybe there should be migration removing target deprecated coins accounts from state as well. but its not that big deal. it can stay there forever and nothing will probably ever happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will prepare migration for users with remembered wallet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
2a3c207
to
4bdc0ac
Compare
50973b7
to
0405953
Compare
22960bc
to
bd9da1f
Compare
bd9da1f
to
53e914c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from mentioned, it looks good (= same as e.g. tgor
deprecation).
53e914c
to
2cb1f13
Compare
c9c3719
to
d76c9c1
Compare
As this affects additional files and tests and it is not necessary for the deprecation, I will leave it for later cleanup together with BE deprecation of these coins |
fa490fd
to
3aa96f8
Compare
chore(suite-native): basic deprecation on suite lite level
3aa96f8
to
7ddc5a4
Compare
7ddc5a4
to
b71ef01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 👍. I have tested it and it works as expected 🦾 .
Please, create issue for redundant portfolioTrackerMainnets
and remove that log as you discussed in comments
chore(suite): add migration for deprecated coins
b71ef01
to
666f142
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 👏 The only issue is that you don't remove enough coins, I would delete a few more 😉
One nitpick is that there is missing migration for graph state (we stored the preferred time range per coin), but it's not deleted properly even if you just disable any coin, so I would keep it as it is. It is quite hard to filter those and it's just a small data, most users will not have it anyway...)
Works as expected, I was checking the redux state after migration from develop to this branch.
As of February 2025, Trezor Suite will discontinue support for Dash, Bitcoin Gold, DigiByte, Namecoin, and Vertcoin. Users are advised to migrate their funds to alternative wallets to ensure continued access and security. The coins will still be supported on the FW level, so users can still access them using third party wallets.
Proposed Timeline:
I will divide the commits by environment (desktop/native) to make code review easier.
Please read the following before conducting the code review:
Given the points above, I have prepared only basic deprecation that mainly affects NetworksConfig. Everything else (tests, icons, etc.) will remain unchanged until we decide to disable trading options and/or turn off the backends.
Todo:
messages.ts
and other mentions in general files?NetworksConfig
?This pull request includes significant changes related to the removal of support for several cryptocurrencies across multiple files. The most important changes include updating the
LegacyNetworkSymbol
type, modifying protocol definitions, and removing network configurations and symbols.Removal of support for specific cryptocurrencies:
packages/product-components/src/components/CoinLogo/coins.ts
: UpdatedLegacyNetworkSymbol
to include additional unsupported coins.suite-common/suite-constants/src/protocol.ts
: Removed protocols forbitcoingold
,dash
,digibyte
,namecoin
, andvertcoin
. [1] [2]suite-common/wallet-config/src/networksConfig.ts
: Removed network configurations fordash
,btg
,dgb
,nmc
, andvtc
.suite-common/wallet-config/src/types.ts
: UpdatedNetworkSymbol
to remove symbols forbtg
,dash
,dgb
,nmc
, andvtc
.suite-native/config/src/supportedNetworks.ts
: Updated the whitelist and removed the blacklist for unsupported coins. [1] [2]